Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add configuration for DESPIAD project #572

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Add configuration for DESPIAD project #572

wants to merge 35 commits into from
+622 −13

Conversation

p-j-smith
Copy link
Contributor

@p-j-smith p-j-smith commented Dec 12, 2024 •
edited by stefpiatek
Loading

Description

Closes #571
Closes #530

  • add project config for DESPIAD
  • add tag operation files for CT and PET
  • use base, CT, then PET tag operation files for anonymisation
  • ensure all resources sharing a StudyInstanceUID in Orthanc Raw are exported (they may exist as separate resources in Orthanc Raw if they have different StudyIDs, but are combined into a single resource in Orthanc Anon as the StudyID tag is deleted) This has already been fixed in Improve anon logging and anonymise all matching studies at once #582

Type of change

Please delete options accordingly to the description.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Suggested Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have passed on my local host device. (see further details at the CONTRIBUTING document)
  • Make sure your branch is up-to-date with main branch. See CONTRIBUTING for a general example to syncronise your branch with the main branch.
  • I have requested review to this PR.
  • I have addressed and marked as resolved all the review comments in my PR.
  • Finally, I have selected squash and merge

@p-j-smith
Add initial anonymisation config for ct
54eb64f
@p-j-smith
Add initial anonymisation config for pet
2730317
@p-j-smith
Add config for despiad
ff2a8c3
@p-j-smith
anonymnise all resources before notifying the export api
f20d5b8 
this is because resources sharing the same StudyInstanceUID in Orthanc Raw will be combined into a single resource in Orthanc Anon.
Previously, we would try to export each resource after anonymisation, but this meant other resources sharing the same StudyInstanceUID were not exported.
@codecov Codecov
Copy link

codecov bot commented Dec 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.34483% with 23 lines in your changes missing coverage. Please review.

Project coverage is 83.47%. Comparing base (4afca8a) to head (09748bf).

Files with missing lines Patch % Lines
pixl_dcmd/src/pixl_dcmd/main.py 42.42% 19 Missing ⚠️
..._core/src/core/project_config/pixl_config_model.py 84.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
+ Coverage   75.73%   83.47%   +7.74%     
==========================================
  Files          56       72      +16     
  Lines        2811     3420     +609     
==========================================
+ Hits         2129     2855     +726     
+ Misses        682      565     -117

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@p-j-smith p-j-smith marked this pull request as ready for review January 15, 2025 10:34
p-j-smith
p-j-smith commented Jan 15, 2025
View reviewed changes
Copy link
Contributor Author

@p-j-smith p-j-smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know the allowed_manufactures we should add for each project?

pixl_dcmd/tests/test_main.py Outdated
@@ -194,7 +194,9 @@ def ids_for_parameterised_test(val: pathlib.Path) -> str:


@pytest.mark.parametrize(
("yaml_file"), PROJECT_CONFIGS_DIR.glob("*.yaml"), ids=ids_for_parameterised_test
("yaml_file"),
PROJECT_CONFIGS_DIR.glob("test-*.yaml"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer testing anonymisation using the non-test project configs. If we want to test all project configs, we need to add:

allowed_manufacturers:
  - regex: "^company"
    exclude_series_numbers: []

to all configs, unless there's a nicer way to do it?

Copy link
Contributor

@stefpiatek stefpiatek Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the existing projects we can just allow all manufacturers, as that's what was implicitly done. We do have a test that ensures all configs are valid though right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that's fair, and for DESPIAD shall we limit to known manufacturers? And yeah we have a separate test to check configs are valid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've allowed all manufacturers for the existing configs, and added them back to the test_anonymise_and_validate_dicom tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefpiatek stefpiatek stefpiatek left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add config for DESPIAD PIXL pipeline for CT data
3 participants
@p-j-smith @stefpiatek @davecash75